-
Notifications
You must be signed in to change notification settings - Fork 538
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TextInput: Add LeadingVisual
/TrailingVisual
to be part of the accessible description in TextInput
#4939
Conversation
🦋 Changeset detectedLatest commit: e9d65a2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
LeadingVisual
text to be accessible description of TextInput
LeadingVisual
/TrailingVisual
to be part of the accessible description in TextInput
/** Text for screen readers to convey the loading state */ | ||
loaderText?: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 I wonder if it's really necessary / a good idea for consumers to be able to customize this? It inherently means they'll be providing a different experience for sighted vs non-sighted users, because sighted users won't be able to access this text. Visually a loading indicator only indicates that something is loading, so "Loading" should be sufficient always.
Alternatively, we could make this visible by putting it in a tooltip. But that doesn't seem ideal since the loader is not focusable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, and good point! This mainly stems from https://github.com/github/primer/issues/3465. I believe the idea is, if there's context outside of the input that conveys what is loading then it might be a good idea to add more descriptive text to indicate what is loading so users don't have to navigate outside of it to get that context.
I think this is a more specific use case that won't be used unless necessary, so in most cases having the default "Loading" is okay, which we could make more descriptive 🤔
Cc: @patrickhlauke - curious if you have any additional thoughts here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the idea is, if there's context outside of the input that conveys what is loading then it might be a good idea to add more descriptive text to indicate what is loading so users don't have to navigate outside of it to get that context.
If we do keep this prop, I think this (particularly the par about the context outside the input) seems like it would be useful to add to the props documentation. I think what may happen is consumers will see that this prop exists and think that they should fill it out similarly to how they fill out alt text in an image - they'll have good intentions but actually create a less accessible experience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, that's totally fair @iansan5653! I can add a note on this in the props docs to make sure consumers aren't haphazardly using it!
Co-authored-by: Ian Sanders <iansan5653@github.com>
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/347575 |
🟢 golden-jobs completed with status |
Closes https://github.com/github/primer/issues/3438 https://github.com/github/primer/issues/3465
Changelog
New
aria-describedby
forLeadingVisual
andTrailingVisual
loaderText
to convey loading state to screen readersRollout strategy
Testing & Reviewing
Merge checklist
(https://github.com/github/github/pull/341243, integration tests pass with minor changes, and some unrelated failures)